Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Turned warnings into errors #89

Merged

Conversation

nicoddemus
Copy link
Member

  • Fix two correct warnings
  • Used pytest.warns() around a valid warning being issued

Should remove the warning exception for #81 when we tackle that.

Fix #78

- Fix two correct warnings
- Used pytest.warns() around a valid warning being issued

Should remove the warning exception for pytest-dev#81 when we tackle that.

Fix pytest-dev#78
@@ -675,7 +675,8 @@ def __call__(self, *args, **kwargs):
warnings.warn(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guys shouldn't we turn this into a error right away instead of just a warning? IIUC the effect of this is things silently working because the missing argument is not required by any of the impls, but as soon as a new plugin is registered then everything blows up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but as soon as a new plugin is registered then everything blows up.

Only if the hook is called with less then the spec's args and some hookimpl is trying to accept those new args.
That will happen anyway; this code makes no difference.

IIRC this was introduced is to alert hook callers of newly introduced args without enforcing they're provided in calls (yet). An example might be where a plugin supports hook calling multiple times but newer versions should be called with additional args. An example might be in pytest where you want to add an additional arg to say pytest_runtestloop but don't want to break anyone's code who implements pytest_cmdline_main and already calls that function themselves. IIRC this came from the initial attempt at solving #15 and I think I pulled this out and PR-ed it in separately. I think this came from the idea of "graceful introduction" of new arguments.

I'm all for scrapping it though if it's not making any sense.
@RonnyPfannschmidt your thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if the hook is called with less then the spec's args and some hookimpl is trying to accept those new args.
That will happen anyway; this code makes no difference.

Indeed it will happen, but I would expect it to happen sooner (as soon as the new spec argument was introduced) than later (only after a hookimpl which needs it).

But now that we mention #15 it makes sense, because then the hookiml will receive the default value defined in the spec, instead of things blow up.

If this is a half-way measure to get there I think we are fine then.

Thanks for the explanation! 😉

Copy link
Contributor

@goodboy goodboy Sep 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed it will happen, but I would expect it to happen sooner (as soon as the new spec argument was introduced) than later (only after a hookimpl which needs it).

Right it would just break client code immediately then - which I'm also totally cool with.
Previously this was being ignored without the error anyway.

I wonder what @RonnyPfannschmidt thinks?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

breaking client code right away is not something that should be done right away if it can be avoided

change and breaking change is necessary to keep the world moving, but unexpected hard breakage should be avoided unless the cost of doing so is rather unreasonable

filterwarnings =
error
# inspect.getargspec() ignored, should be fixed in #81
ignore:inspect.getargspec().*deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice did not know you could do that 👍

Copy link
Contributor

@goodboy goodboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job!

@RonnyPfannschmidt RonnyPfannschmidt merged commit 6f28308 into pytest-dev:master Sep 13, 2017
@nicoddemus nicoddemus deleted the warnings-into-errors branch September 13, 2017 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants